Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit/issues #23

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Audit/issues #23

wants to merge 2 commits into from

Conversation

kmankan
Copy link
Member

@kmankan kmankan commented Dec 28, 2024

Important

Improves logging and adjusts code comments for selection sort and server URL logic in backend and frontend.

  • Logging Improvements:
    • Enhanced logging in index.ts for selection sort and DFS routes to include request and response details.
    • Added network connection log in express.ts to check server accessibility.
  • Code Adjustments:
    • Commented out console log in selectionSort.ts.
    • Corrected comment in SelectionSort.tsx to reference selectionSort instead of bubbleSort.
  • Server URL Logic:
    • Added selectServerUrl function in express.ts to dynamically choose between deployed and local server URLs based on network connection.

This description was created by Ellipsis for 76dca74. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 76dca74 in 1 minute and 28 seconds

More details
  • Looked at 86 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/index.ts:76
  • Draft comment:
    Typo in console log message: 'recieved' should be 'received'. This typo is present in multiple places.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The console log message has a typo in 'recieved'. This typo is present in multiple places and should be corrected for clarity and professionalism.
2. backend/index.ts:77
  • Draft comment:
    Typo in console log message: 'recieved' should be 'received'. This typo is present in multiple places.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The console log message has a typo in 'recieved'. This typo is present in multiple places and should be corrected for clarity and professionalism.

Workflow ID: wflow_y4YaxvfAJW3OuY4A


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

const DEPLOYED_SERVER_URL = 'https://algorithm-visualisations-express-production.up.railway.app'

async function selectServerUrl() {
const hasNetworkConnection = await axios({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The network connectivity check is incorrect. hasNetworkConnection will always be truthy if the request doesn't throw an error. Consider using a try-catch block to handle errors and determine connectivity.

}

// It's not clear to me why this promise is getting rejected?
const TEST_URL = selectServerUrl()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEST_URL is assigned a promise instead of the resolved value. Use await selectServerUrl() to get the resolved value.

const TEST_URL = selectServerUrl()
console.log('server', TEST_URL)

const SERVER_URL = 'http://localhost:8080'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SERVER_URL is hardcoded to 'http://localhost:8080'. Consider using the result from selectServerUrl() to dynamically set the server URL based on connectivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant